Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

clients(devtools): tsc type override bug workaround #12933

Merged
merged 2 commits into from
Aug 18, 2021

Conversation

connorjclark
Copy link
Collaborator

split off from #12926

@connorjclark connorjclark requested a review from a team as a code owner August 18, 2021 19:22
@connorjclark connorjclark requested review from adamraine and removed request for a team August 18, 2021 19:22
@google-cla google-cla bot added the cla: yes label Aug 18, 2021
report/README.md Outdated
Comment on lines 55 to 61
However, this breaks tsc declaration creation, specifically needed for the DevTools bundle. It would created something like:

```js
export type DOM = any;
// ...
export class DOM { ... }
```
Copy link
Member

@brendankenny brendankenny Aug 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we fix this in bundle.esm.js? We could look at the named exports and strip out any typedef {import(...)}s of the same name. In theory more maintainable than noticing when yarn test-devtools fails and trying to figure out why.

Looking inside bundle.esm.js, though, I don't entirely understand why that /** @typedef {import('./dom.js').DOM} DOM */ isn't stripped from the inlined components.js but it is stripped from the inlined report-renderer.js.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the contents of bundle.esm.js aren't of concern here, but bundle.esm.d.ts (which is only generated in the roll script, heh), but your suggestion is still applicable. I believe we might be able to just delete export type DOM = any; after tsc makes the file and everything will be hunky dory.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mentioned bundle.esm.js because that's what bundle.esm.d.ts is generated from and I believe why bundle.esm.d.ts ends up with both a type and a class DOM.

But why bundle.esm.js has both a typedef and a class DOM in the first place (and not zero DOM typedefs or many DOM typedefs), I don't know, so this works for me :) 👍

@connorjclark connorjclark changed the title report: document typedef/import bug workaround clients(devtools): tsc type override bug workaround Aug 18, 2021
@connorjclark connorjclark merged commit a1dfe6c into master Aug 18, 2021
@connorjclark connorjclark deleted the report-typedef-vs-import branch August 18, 2021 22:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants